-
Notifications
You must be signed in to change notification settings - Fork 640
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix nunjucks for windows: relative paths and tests #391
Conversation
The main problem with unit tests is that we are comparing strings that include This is failing on Windows because it's replaced by @jlongster Should I adapt the tests to use Or we can just normalize tests by wrapping the render with |
@SamyPesse hm that's tricky. Any chance you know what other projects do? Is it safe to normalize it? |
This is maybe safer to just normalize tests for now, for example at https://github.com/mozilla/nunjucks/blob/master/tests/compiler.js#L326:
with:
ok ? |
That works for me! |
I'd say we only need to do it in tests where we need to compare the |
Tests are now working on Windows, I'm testing setup tests on Appveyor (for my fork). the use of |
BTW here are the builds that I'm testing using AppVeyor: https://ci.appveyor.com/project/SamyPesse/nunjucks |
There is a very important issue in Mocha for windows that has been fixed in a merged PR but not yet release to NPM: mochajs/mocha#333 (comment) |
What the issue we experience because of that bug? They say they won't backport to 0.10 but I still want to support that. |
Basically stdout and stderr are not flushed and so not visible in the build logs. The only other workaround is to use the I'm running some tests, but I fix this is not important if logs are not available for some windows builds, it's enough to know that it's failing and getting the logs for node 0.11, because if it's failing only for 0.10/0.8, it will probably fail for the same reason on travis (where the logs are always available). Sorry if my comment is not clear enough, I'm not an english native :) |
🎉 🎈 🚀 Tests are passing on AppVeyor: https://ci.appveyor.com/project/SamyPesse/nunjucks/build/job/94ovau3gea1co8nr ! At least on 0.11, I'm still waiting for 0.8 and 0.10: https://ci.appveyor.com/project/SamyPesse/nunjucks |
@jlongster this is ready to be merged :) I disabled windows builds for node 0.8 because it was always failing without being related to Nunjucks: https://ci.appveyor.com/project/SamyPesse/nunjucks/build/1.0.3/job/nd0nj203yqeods0c You can signup to appveyor and create a project for nunjucks if you want to enable windows testing, otherwise at least it's ready for it :) |
👍 |
@@ -52,6 +52,11 @@ | |||
} | |||
} | |||
|
|||
function normEOL(str) { | |||
if (!str) return str; | |||
return str.replace(/\r\n|\r|\n/g, "\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\n
needn't be replaced, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean using /\r\n|\r/g
instead? I think it will work fine, yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I'll replace it
LGTM |
Fix nunjucks for windows: relative paths and tests
Thanks 👍 🍻 |
@SamyPesse thanks for your work on it! As far as Appveyor is concerned, is your project here (https://ci.appveyor.com/project/SamyPesse/nunjucks) testing mozilla's project or your local fork? I added Mozilla's repo but its showing up under my name: https://ci.appveyor.com/project/jlongster/nunjucks We can keep it under your name if its testing this repo. I'll add a link to the README. |
It's testing my fork, I can't enable it to test this repo (since I don't have access to the repo webhooks). |
https://ci.appveyor.com/project/jlongster/nunjucks should work now. @jlongster you can register a mozilla account if want the link http://help.appveyor.com/discussions/questions/744-using-appveyor-for-a-github-org |
Ok, thanks. I'll keep it under jlongster because I don't want to manage the mozilla account at Appveyor. As long as it's testing this repo. |
Any news on when the latest updates will be available on NPM? :) |
Because of some of the bugs we found I wanted to make sure there wasn't anything else outstanding with this. And I got busy recently. I will put some time into it this week though and make a release if it looks stable. |
@SamyPesse just pushed out v1.3.0. Thanks for all your work! Is there any chance you could add documentation for the features you added in the /docs folder? Particularly for relative templates, but also for the |
The goals of this PR are:
\n
and on windows it's\r\n
)